-
Notifications
You must be signed in to change notification settings - Fork 129
Implement WAFPolicy controller #3532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/nap-waf
Are you sure you want to change the base?
Conversation
e3a3e51
to
89122cd
Compare
89122cd
to
469bfd8
Compare
internal/controller/nginx/config/policies/wafsettings/generator.go
Outdated
Show resolved
Hide resolved
469bfd8
to
77a793f
Compare
ddeaa8e
to
499cfed
Compare
499cfed
to
752013c
Compare
ad59974
to
047418f
Compare
047418f
to
28bcbe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple small things, but overall looks pretty good!
if wp.Spec.PolicySource != nil && wp.Spec.PolicySource.FileLocation != "" { | ||
fileLocation := wp.Spec.PolicySource.FileLocation | ||
bundleName := helpers.ToSafeFileName(fileLocation) | ||
bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First arg is a string literal, doesn't seem like we need to format it.
|
||
if secLog.LogProfileBundle != nil && secLog.LogProfileBundle.FileLocation != "" { | ||
bundleName := helpers.ToSafeFileName(secLog.LogProfileBundle.FileLocation) | ||
bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
}, | ||
expStrings: []string{ | ||
"app_protect_enable on;", | ||
"app_protect_policy_file \"/etc/app_protect/bundles/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the file name be a part of this?
func (v Validator) Conflicts(polA, polB policies.Policy) bool { | ||
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polA) | ||
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polB) | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (v Validator) Conflicts(polA, polB policies.Policy) bool { | |
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polA) | |
_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polB) | |
return false | |
} | |
func (v Validator) Conflicts(_, _ policies.Policy) bool { | |
return false | |
} |
if policyKey.GVK != wafPolicyGVK { | ||
continue | ||
} | ||
|
||
if !policy.Valid { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could combine these two
Proposed changes
Problem:
As a user of NGF
I want my WafPolicy configuration applied to NGINX for the Gateway or Route scope
So that I can enable WAF protection on my traffic
Solution: Implement the WAFPolicy controller.
Testing: Over 90% unit test coverage, and manual testing in a GKE cluster
Closes: #3454
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.